feat: parse Q10 archived map packets - #936
Conversation
| erase_zones = _parse_erase_zones(tail) | ||
| carpet_mask = _parse_carpet_mask(tail, width, height) | ||
| carpet_mask, carpet_end = _parse_carpet_block(tail, width, height) | ||
| if kind is Q10MapPacketKind.CLEAN_RECORD_DETAIL and carpet_end is not None: |
There was a problem hiding this comment.
when this kind of map packet is received, how many of these other fields are also included?
What i'm wondering is if a historical trace kind has a lot of overlap with other fields in Q10MapPacket or if it needs to be a separate type. basically as more fields are added the map packet seems like a sparse object. You could imagine each kind Q10MapPacketKind has a separate dataclass for example, if the overlap is low.
There was a problem hiding this comment.
re-reading the code I don't actually see the historical trace even being used anywhere yet, except in the tests.
Naively, it seems to me like we shouldn't be sticking this on to a map package and instead just using the kind to parse a clean record?
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
I am very happy to keep momentum going on this. I am adding additional feedback since this hasn't been moving since my last comment, in case it was missed. I have a lot of time to review during the us holidy weekend.
| if Q10MapPacketKind.from_payload(payload) is Q10MapPacketKind.TRACE: | ||
| return parse_trace_packet(payload) | ||
| if Q10MapPacketKind.from_payload(payload) is not None: | ||
| return parse_map_packet(payload) |
There was a problem hiding this comment.
How about we get the kind once:
| if Q10MapPacketKind.from_payload(payload) is Q10MapPacketKind.TRACE: | |
| return parse_trace_packet(payload) | |
| if Q10MapPacketKind.from_payload(payload) is not None: | |
| return parse_map_packet(payload) | |
| kind = Q10MapPacketKind.from_payload(payload) | |
| if kind is Q10MapPacketKind.TRACE: | |
| return parse_trace_packet(payload) | |
| if kind is not None: | |
| return parse_map_packet(payload) |
|
|
||
|
|
||
| def lz4_block_decompress(data: bytes) -> bytes: | ||
| def lz4_block_decompress(data: bytes, max_output_size: int | None = None) -> bytes: |
There was a problem hiding this comment.
No need to make the second arg optional given we're updating all the callers in this change i believe.
| @@ -84,7 +84,7 @@ class Q10MapOverlays: | |||
|
|
|||
| def render_q10_map( | |||
There was a problem hiding this comment.
Whats the relationship between a Q10MapPacket and a trace? especially Q10HistoricalTracePacket.
My impression is Q10HistoricalTracePacket is a field that was added to Q10MapPacket, so why would it also be passed here as trace?
| carpet_mask = _parse_carpet_mask(tail, width, height) | ||
| carpet_mask, carpet_end = _parse_carpet_block(tail, width, height) | ||
| if kind is Q10MapPacketKind.CLEAN_RECORD_DETAIL and carpet_end is not None: | ||
| historical_trace, _ = _parse_clean_record_trace(tail, carpet_end) |
There was a problem hiding this comment.
we just ignore the second return value, so can it be removed?
| erase_zones = _parse_erase_zones(tail) | ||
| carpet_mask = _parse_carpet_mask(tail, width, height) | ||
| carpet_mask, carpet_end = _parse_carpet_block(tail, width, height) | ||
| if kind is Q10MapPacketKind.CLEAN_RECORD_DETAIL and carpet_end is not None: |
There was a problem hiding this comment.
re-reading the code I don't actually see the historical trace even being used anywhere yet, except in the tests.
Naively, it seems to me like we shouldn't be sticking this on to a map package and instead just using the kind to parse a clean record?
2 participants
Summary
01/02/03/04archive packet markers with a typed integer enumImportant
This is a stacked pull request based on #933. The isolated change for review is commit
95d268e. After #933 merges, this branch will be rebased ontomainbefore merge so the final diff and history contain only this change.Design against the #933 review priorities
Q10MapPacketmodel for current and archived maps, so consumers do not inspect raw packet markers.Review notes
This split incorporates the earlier review direction on #933: packet kinds own their wire integer values, the packet kind is required and parsed first, and unused trailing packet bytes are not exposed as public state.
No private map captures or account data are included.
Validation
981 passed, including92snapshot testsRelated work
AI assistance disclosure
This contribution was prepared with OpenAI Codex assistance. I reviewed the submitted changes and test results and take responsibility for the contribution.